hotfix: serialize resumptions to prevent cancel+restore race condition #9025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes concurrent resumption flows that can cause 'two instances' behavior when: - UI cancel schedules presentResumableAsk via setImmediate - Webview checkpoint restore calls cancel then restore - Both flows reset abort flags and call into task loop simultaneously Changes: - Add suppressResumeAsk flag in ClineProvider to gate scheduling during transactions - Add resumeAskScheduledForTaskId dedupe to prevent multiple scheduled asks per task - Wrap checkpoint restore flow with suppression to serialize with cancel path - Enhanced scheduling guards: re-check suppression, verify task not switched/abandoned - Unit tests for gating behavior and deduplication Addresses PR #8986 recommendations for provider-level resumption serialization.
dosubot
bot
added
size:L
|
Reviewed the race condition fix. Found 1 issue that needs to be addressed:
Mention @roomote in a comment to trigger your PR Fixer agent and make changes to this pull request. |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
Comment on lines
1054
to
1058
| } finally { | ||
| // End transaction and post consolidated state | ||
| await provider.endStateTransaction() | ||
| provider.setSuppressResumeAsk(false) | ||
| } |
There was a problem hiding this comment.
If endStateTransaction() throws an exception, setSuppressResumeAsk(false) on line 1057 won't execute, leaving the suppression flag permanently set to true. This would prevent all future resume asks from being scheduled until VS Code is restarted. Nest the cleanup in a second try-finally block to guarantee the flag is always reset:
} finally {
try {
await provider.endStateTransaction()
} finally {
provider.setSuppressResumeAsk(false)
}
}Fix it with Roo Code or mention @roomote and request a fix.
hannesrudolph
added
the
Issue/PR - Triage
|
Reviewing your PR now. Feedback coming soon! Mention @roomote in a comment to trigger your PR Fixer agent and make changes to this pull request. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes concurrent resumption flows that can cause two instances behavior when UI cancel schedules presentResumableAsk via setImmediate and webview checkpoint restore calls cancel then restore. Both flows can reset abort flags and call into task loop simultaneously.
Solution: Add suppressResumeAsk flag in ClineProvider to gate scheduling during transactions, plus resumeAskScheduledForTaskId dedupe to prevent multiple asks per task. Wrap checkpoint restore flow with suppression to serialize with cancel path.
Testing: Extended existing tests and added new test for checkpoint restore suppression. All 10 tests across 4 files pass.
Related to: #8986
Addresses PR 8986 recommendations for provider-level resumption serialization.
Important
Introduces suppression and deduplication in
ClineProviderto prevent concurrent task resumptions, with tests added for validation.suppressResumeAskflag andresumeAskScheduledForTaskIdinClineProviderto prevent concurrent resumptions.webviewMessageHandler.tswith suppression to serialize with cancel path.cancelTask()inClineProviderto checksuppressResumeAskand deduplicatepresentResumableAskscheduling.webviewMessageHandler()to setsuppressResumeAskduring checkpoint restore.ClineProvider.cancelTask.present-ask.spec.tsfor suppression and deduplication logic.webviewMessageHandler.resume-gating.spec.tsto test suppression during checkpoint restore.This description was created by
for 7827cb7. You can customize this summary. It will automatically update as commits are pushed.